Fix bug #69168: DomNode::getNodePath() returns invalid path #10318
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nielsdos
changed the title
nielsdos
changed the title
cmb69
reviewed
Jan 15, 2023
There was a problem hiding this comment.
Thank you!
That's a great analysis, and the fix looks good!
Targeting master because this technically is a BC break because the original node gets returned instead of a copy, so modifications will now persist.
Makes sense to me to target master, especially because the issue is quite old.
Upon freeing libxslt's context, every document which is not the *main* document will be freed by libxslt. If a node of a document which is not the main document gets returned to userland, we'd free the node twice: - first by the cleanup of the xslt context - and then by our own refcounting mechanism. This was reported in bug #49634, and was fixed by always copying the node (and later re-fixed in bug #70078). The original fix is not entirely correct unfortunately because of the following two main reasons: - modifications to the node will only modify the copy, and not the original - accesses to the parent, path, ... will not work This patch fixes it properly by only copying the node if it origins from a document other than the main document.
This additionally tests for modifications of nodes. Co-authored-by: juha.ikavalko@agentit.fi
|
Fixed the codestyle :) Thanks. |
|
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes bug #69168 (old bug tracker)
The root cause of this bug is that we're copying the node, which breaks the connection to the parent. So getting the parent or getting the path doesn't work.
The reason for the copy is the following:
Upon freeing libxslt's context, every document which is not the main document will be freed by libxslt.
If a node of a document which is not the main document gets returned to userland, we'd free the node twice:
This was reported in bug #49634 (old bug tracker), and was fixed by always copying the node (and later re-fixed in bug #70078 (old bug tracker)).
The original fix is not entirely correct unfortunately because of the following two main reasons:
This patch fixes it properly by only copying the node if it origins from a document other than the main document.
And by doing so, consequently fixes bug #69168 (old bug tracker).
Targeting master because this technically is a BC break because the original node gets returned instead of a copy, so modifications will now persist.
As an additional plus to this PR, the transformation will now use less memory.
Requesting a review from @cmb69 because he previously fixed a bug in this function and also verified the bug report.
EDIT: failure in Travis is from
proc_open [ext/standard/tests/general_functions/proc_open02.phpt]again, which is unrelated.